Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial code to support llvm-20 #684

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

KaruroChori
Copy link

@KaruroChori KaruroChori commented Feb 6, 2025

I started working on support for llvm20, since the interface changed in some places.
At the moment there are few things still missing which I have not been able to figure out:

terra/src/tcwrapper.cpp:831:35: error: no matching function for call to ‘clang::CompilerInstance::createDiagnostics()’

as the interface of createDiagnostics had some significant changes and is now based on a virtual file system (?)

terra/src/tllvmutil.cpp:120:9: error: ‘ExtraVectorPassManager’ was not declared in this scope

as this structure totally disappeared from upstream for what I can tell. No idea on which replacement was designed.

Finally, a minor issue with CloneBasicBlock which saw an argument removed and I am not sure if I patched it up correctly.

@KaruroChori
Copy link
Author

KaruroChori commented Feb 6, 2025

I also tested llvm-19. That one compiles just fine with these changes, but running it ends up with the infamous message:

: CommandLine Error: Option 'o' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

I built it against a custom compiled llvm-19 following similar steps as in #678, so I am not sure what is causing that.

@elliottslaughter
Copy link
Member

Thanks for working on this! I don't have much bandwidth to help at the moment, but I'll try to answer questions where I can.

terra/src/tllvmutil.cpp:120:9: error: ‘ExtraVectorPassManager’ was not declared in this scope

If you scroll up you'll see the comment says it comes from PassBuilder::addVectorPasses, which is a non-public LLVM interface. Unfortunately, LLVM decided not to expose this part of their pass builder. And (as happens too often in LLVM-land) they removed the old interface without a proper replacement.

I submitted an issue asking for them to make a public version, which they seem to have ignored: llvm/llvm-project#67076

It's too bad that there has been churn here. But unless you're aware of a different solution, we're stuck with copy-and-pasting the function and ifdef-ing it as necessary to make it apply to all versions.

@elliottslaughter
Copy link
Member

elliottslaughter commented Feb 6, 2025

I also tested llvm-19. That one compiles just fine with these changes, ...

We could potentially take a version of this PR with only LLVM 19 changes. That would be personally useful to me (and maybe other members of the community).

but running it ends up with the infamous message:

: CommandLine Error: Option 'o' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

I built it against a custom compiled llvm-19 following similar steps as in #678, so I am not sure what is causing that.

Unfortunately I'm still not sure what to say about this. I imagine it's not really version-related. I could build LLVM 19 on my end, but I don't think that would fix it?

@KaruroChori
Copy link
Author

KaruroChori commented Feb 6, 2025

I could build LLVM 19 on my end, but I don't think that would fix it?

Could you try? Just so that I can rule out if it is something related to my environment. In which case I can probably narrow down what is going on a bit.

@KaruroChori
Copy link
Author

KaruroChori commented Feb 6, 2025

And (as happens too often in LLVM-land) they removed the old interface without a proper replacement.

Well, I am not surprised :D. Thank you for these details.

Sadly I know very little of llvm internals, so I can try to work something out for few hours, but I will bail out and ignore llvm20 if I see it is creeping too much :).

To be honest, I dream one day of something like terra but based on a more lightweight backend, like qbe. At least, if I find bugs or regressions, the code-base is tame enough I can solve them and patch upstream. I cannot say the same of llvm :(.

@KaruroChori
Copy link
Author

KaruroChori commented Feb 7, 2025

All reports I found suggest this problem is due to llvm being double linked somewhere.
The result is that one of the global constructors gets to run twice on the same data, and it complains of that the second time.
For example AdaptiveCpp/AdaptiveCpp#1403 but there are several other instances more or less equivalent to that.

@elliottslaughter
Copy link
Member

LLVM 19 binaries are here: https://github.com/terralang/llvm-build/releases/tag/llvm-19.1.7

I pushed changes into this branch to enable LLVM 19 CI on Linux, Mac and FreeBSD.

Looks like we're hitting the same failure mode: https://github.com/terralang/terra/actions/runs/13205235502/job/36866760732?pr=684

So I guess the next step is for me to debug this locally.

@elliottslaughter
Copy link
Member

To be honest, I dream one day of something like terra but based on a more lightweight backend, like qbe. At least, if I find bugs or regressions, the code-base is tame enough I can solve them and patch upstream. I cannot say the same of llvm :(.

Honestly, I agree. I don't know this backend specifically, but if we had a backend that was stable, well-supported, and didn't make Terra harder to maintain, I'd be happy to merge it. There are things I can only do with LLVM, but keeping up with LLVM updates is such a pain.

All reports I found suggest this problem is due to llvm being double linked somewhere.

Now that it's replicated I'll start debugging it. In the past it was related to mixing static and dynamic objects. But you can see in the binaries yourself that there are no .so files...

@elliottslaughter
Copy link
Member

You can see my initial attempt to fix this in 6cb9edb. My hunch there was that LLVM was accidentally including either (a) some object, or (b) some specific item in multiple .a files. Since we force-load those, it would cause the option to be linked twice, thus the conflict.

I currently see two problems.

First, Linux is still broken and requires an additional patch to work. The patch is here:

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 1a8e114..6a4f4de 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -347,16 +347,34 @@ elseif(UNIX)
         "-Wl,-rpath,$ORIGIN/../lib"
     )
   endif()
+  if(TERRA_WHOLE_ARCHIVE_LLVM)
+    target_link_libraries(TerraExecutable
+      PRIVATE
+        -Wl,-export-dynamic
+        -Wl,--whole-archive
+    )
+  endif()
+
   target_link_libraries(TerraExecutable
     PRIVATE
-      -Wl,-export-dynamic
-      -Wl,--whole-archive
       TerraLibrary
       -Wl,--no-whole-archive
       ${LUAJIT_LIBRARIES}
       ${ALL_LLVM_LIBRARIES}
       ${LLVM_SYSTEM_LIBRARIES}
   )
+  if(TERRA_WHOLE_ARCHIVE_LLVM)
+    target_link_libraries(TerraExecutable
+      PRIVATE
+        -Wl,--no-whole-archive
+    )
+  endif()
+  target_link_libraries(TerraExecutable
+    PRIVATE
+      ${LUAJIT_LIBRARIES}
+      ${ALL_LLVM_LIBRARIES}
+      ${LLVM_SYSTEM_LIBRARIES}
+  )
 else()
   target_link_libraries(TerraExecutable
     PRIVATE

But I have not pushed this patch because it actually does not fix the issue. On macOS (and Linus with this patch), I get:

llvm: JIT has not been linked in.

This makes me think that we really do need the whole-archive link. However, maybe we need to exclude whole-archive from specific LLVM .a libraries. That would be a massive pain, but might be what we're stuck with. At any rate, it means my fix in 6cb9edb may not actually be the correct solution, and my addition patch above for Linux might not be great either. We probably need, like I said, to hunt down exactly where the duplicate symbol is coming from and exclude only that .a from the forced load.

Unfortunate I've exceeded my available debugging time so that's all I can do for the moment.

If you find more time to work on this, feel free to push patches, and I'll approve those to run so you can see CI results.

@KaruroChori
Copy link
Author

KaruroChori commented Feb 8, 2025

Honestly, I agree. I don't know this back-end specifically, but if we had a back-end that was stable, well-supported, and didn't make Terra harder to maintain, I'd be happy to merge it. There are things I can only do with LLVM, but keeping up with LLVM updates is such a pain.

I would not consider it a fair replacement/integration to be honest. It is way less feature rich (by design) and would break any downstream code that is rightfully based on llvm intrinsics as it does not strive for any llvm compatibility.
At best it could make for some kind of "what-if" implementation, reusing a good chunk of terra's own codebase.

Thanks for your tests, I will give it a second try next week. Hopefully I can sort something out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants